Skip to content

Subnet Protocol Alpha Accounting#2645

Open
JohnReedV wants to merge 15 commits into
devnet-readyfrom
chain-buy-cache
Open

Subnet Protocol Alpha Accounting#2645
JohnReedV wants to merge 15 commits into
devnet-readyfrom
chain-buy-cache

Conversation

@JohnReedV
Copy link
Copy Markdown
Contributor

Description

This PR adds protocol-owned alpha accounting for subnet chain buys and includes that alpha in subnet deregistration settlement.

  • Stops recycling alpha bought during protocol TAO buys
  • Adds per-subnet cached protocol alpha accounting
  • Includes protocol alpha-in and cached protocol alpha in deregistration pro-rata settlement
  • Clears cached protocol alpha when a subnet is dissolved
  • Updates tests for the new deregistration settlement behavior

@JohnReedV JohnReedV added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label May 7, 2026
Comment thread pallets/subtensor/src/coinbase/run_coinbase.rs
@github-actions github-actions Bot mentioned this pull request May 21, 2026
4 tasks
gztensor
gztensor previously approved these changes May 26, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 26, 2026

🛡️ AI Review — Skeptic (security review)

VERDICT: SAFE

Baseline scrutiny: established Opentensor contributor with repo write permission; branch chain-buy-cache -> devnet-ready; no trusted Gittensor allowlist hit.

Static review only. The PR does not modify .github/, dependency manifests, lockfiles, or build scripts. I reviewed the protocol-alpha cache, deregistration settlement changes, storage cleanup, arithmetic, generated weight deltas, and contributor/commit signals for malicious behavior, panic surface, origin bypasses, supply-chain risk, hidden activation conditions, and fund-flow security regressions; I did not find a Skeptic-blocking issue.

The known dissolve weight-accounting mismatch remains an Auditor-tracked domain finding, not a Skeptic security finding here because the affected calls are root-only.

Findings

No findings.

Conclusion

No malicious behavior or security vulnerability was found in the reviewed diff.


🔍 AI Review — Auditor (domain review)

VERDICT: 👍

Established opentensor contributor with repo write permission and long subtensor history; Gittensor association UNKNOWN from trusted allowlists.

PR body is substantive and matches the implementation. I did not find a duplicate-work concern among the overlapping open PRs; the overlaps appear to be adjacent subtensor/runtime churn rather than competing implementations.

git diff --check passed. I did not run targeted Rust tests because the carried-forward issue is a static weight-accounting mismatch. Spec-version auto-fix was not applied because the devnet RPC host could not be resolved from this sandbox, so I could not safely compare local spec_version: 412 against live devnet.

Findings

Sev File Finding
MEDIUM pallets/subtensor/src/staking/remove_stake.rs:477 Update dissolve weights for protocol-alpha storage inline

Prior-comment reconciliation

  • 1969a08a: not addressedSubnetProtocolAlpha is still absent from the hard-coded dissolve_network / root_dissolve_network weight accounting; the weights remain reads(6) / writes(31).

Conclusion

The protocol-alpha accounting change is consistent with the PR description and has focused tests for coinbase caching and deregistration settlement. The carried-forward weight-accounting issue remains medium severity because the affected dissolve calls are root-only, so I am not blocking merge on it.


📜 Previous run (superseded)
Sev File Finding Status
MEDIUM pallets/subtensor/src/staking/remove_stake.rs:477 Update dissolve weights for protocol-alpha storage ➡️ Carried forward to current findings
SubnetProtocolAlpha is still absent from the hard-coded dissolve_network / root_dissolve_network weight accounting; the weights remain reads(6) / writes(31).

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +476 to +478
let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid)
.saturating_add(SubnetProtocolAlpha::<T>::get(netuid))
.to_u64() as u128;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Update dissolve weights for protocol-alpha storage

This adds a new SubnetProtocolAlpha read to the dissolve path, and the same function now also removes that map later in the cleanup. The dispatch weights for both dissolve_network and root_dissolve_network in pallets/subtensor/src/macros/dispatches.rs still charge the old reads(6) / writes(31), so the runtime undercharges this path by at least the new protocol-alpha read/write. Please update those annotations or regenerate the relevant weight so the storage proof and DB ops match the new dissolve accounting.

@github-actions
Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +476 to +477
let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid)
.saturating_add(SubnetProtocolAlpha::<T>::get(netuid))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Update dissolve weights for protocol-alpha storage

This adds a SubnetProtocolAlpha read to the dissolve cleanup path, and the same function also removes SubnetProtocolAlpha later. The static weights for dissolve_network and root_dissolve_network in pallets/subtensor/src/macros/dispatches.rs still use reads(6) / writes(31) and do not account for this new storage item. Update those weights at minimum by +1 read/+1 write, or regenerate/replace the hard-coded dissolve weights with a benchmark that exercises SubnetProtocolAlpha.

@github-actions
Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +476 to +477
let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid)
.saturating_add(SubnetProtocolAlpha::<T>::get(netuid))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Update dissolve weights for protocol-alpha storage

destroy_alpha_in_out_stakes now reads SubnetProtocolAlpha here and later removes it, but the dissolve_network / root_dissolve_network dispatch weights are still hard-coded at reads(6) / writes(31). Please add the new read/write to those weights, or re-benchmark and wire the dissolve weight through generated weights, so the root dissolve path charges for the extra storage access.

@github-actions
Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +476 to +477
let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid)
.saturating_add(SubnetProtocolAlpha::<T>::get(netuid))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Update dissolve weights for protocol-alpha storage

do_dissolve_network now reads SubnetProtocolAlpha and later removes it, but the hard-coded weights on both dissolve_network and root_dissolve_network still charge only reads(6) / writes(31). Please account for the extra storage read and write on those dispatch weights. This is root-only, so I am keeping it medium severity rather than blocking as a user-callable weight undercharge.

@github-actions
Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

let mut stakers: Vec<(T::AccountId, T::AccountId, u128)> = Vec::new();
let mut total_alpha_value_u128: u128 = 0;
let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid)
.saturating_add(SubnetProtocolAlpha::<T>::get(netuid))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Update dissolve weights for protocol-alpha storage

This adds protocol-alpha storage access during subnet dissolve, but the call weights for dissolve_network / root_dissolve_network in pallets/subtensor/src/macros/dispatches.rs are still hard-coded at reads(6) / writes(31). The new flow reads SubnetAlphaIn and SubnetProtocolAlpha for the pro-rata denominator and removes SubnetProtocolAlpha during cleanup, so the hard-coded weights under-account the storage work. Update both dissolve call weights to include the added reads/writes, or wire them through a benchmarked WeightInfo entry instead of leaving the stale constants.

@github-actions
Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-cargo-audit This PR fails cargo audit but needs to be merged anyway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants